Open
Conversation
Co-authored-by: geffzhang <439390+geffzhang@users.noreply.github.com>
Co-authored-by: geffzhang <439390+geffzhang@users.noreply.github.com>
Replaces custom skill loading, parsing, and function callback logic with the AgentSkillsDotNet package. Removes legacy code for skill management, utilities, and function callbacks, and introduces new hooks and adapters for integration. Updates dependency references and project files accordingly, and migrates skill definitions and scripts to the new structure.
Major refactor of the Agent Skills plugin to integrate AgentSkillsDotNet, introducing a new SkillService, improved tool registration, and enhanced configuration and security. Adds new hooks for instruction and function injection, restructures plugin initialization, and provides comprehensive documentation, migration guides, and extensive tests and sample skills for validation.
Introduce explicit AgentSkills function callbacks and a controller, and refactor skill service and plugin registration. - Add three IFunctionCallback implementations: GetInstructionsFn, GetSkillBynameFn and GetSkillFileContentFn, plus DTOs GetSkillArgs and SkillFileContentArgs. - Register the new function callbacks in AgentSkillsPlugin and update AgentSkillsFunctionHook to expose corresponding FunctionDef entries for function-based lookup/read operations. - Add SkillController with an authorized GET /skills endpoint to return agent-specific skills. - Refactor ISkillService/SkillService to return per-agent skill lists and remove the previous AITool/AIFunction tool-generation path (including the AIToolCallbackAdapter now commented out and related GetTools/GetAsTools logic removed). - Remove Microsoft.Extensions.AI.Abstractions package from the project file and add small data/config tweaks (agent.json llmConfig and appsettings.json model/provider and ProjectSkillsDir updates). These changes move from dynamic AIFunction tool generation to explicit function callbacks and controller endpoints, and add per-agent skill filtering to better control which skills are exposed.
Remove the AsAITool method from AgentSkill and delete the AgentSkillsAsToolsStrategy enum and ServiceCollectionExtensions files. This cleans up obsolete tools-generation APIs and a convenience service registration helper; update any callers to the new tool/factory patterns as needed.
Contributor
Review Summary by QodoImplement Agent Skills plugin with skill discovery, management, and integration
WalkthroughsDescription* Implements comprehensive Agent Skills plugin for BotSharp with skill discovery, loading, and management capabilities * Adds SkillService for managing agent skills with support for project and user-level skill directories, dynamic reloading, and thread-safe operations * Introduces skill model (AgentSkill) with validation, XML instruction generation, and SKILL.md file parsing with YAML frontmatter support * Implements three skill-related tools: get-available-skills, get-skill-by-name, and read-skill-file-content with security checks and error handling * Adds instruction and function hooks (AgentSkillsInstructionHook, AgentSkillsFunctionHook) for seamless agent integration with progressive disclosure pattern guidance * Extends Agent model with Skills property and updates repository layers (File and MongoDB) for skill persistence * Includes comprehensive test suite with 40+ unit tests, property-based tests using CsCheck, and integration tests covering DI, configuration, and real skill loading scenarios * Adds sample skills (data-analysis, PDF processing, web-scraping, employee-handbook, secret-formulas) demonstrating skill structure and capabilities * Updates dependencies: Microsoft.Extensions.AI.Abstractions to 10.2.0, adds YamlDotNet 16.3.0, and test frameworks (FluentAssertions, CsCheck, coverlet) * Includes detailed documentation: CHANGELOG, test guidelines, property-based testing documentation, and skill directory structure Diagramflowchart LR
A["Agent Model"] -->|"extends with Skills"| B["Agent Skills Plugin"]
B -->|"loads from"| C["Skill Directories"]
C -->|"parses SKILL.md"| D["AgentSkillReader"]
D -->|"creates"| E["AgentSkill Objects"]
E -->|"managed by"| F["SkillService"]
F -->|"provides to"| G["Agent Hooks"]
G -->|"injects instructions"| H["Agent Instructions"]
G -->|"registers tools"| I["Agent Functions"]
I -->|"executes"| J["Skill Tools"]
B -->|"persists to"| K["File/MongoDB Repository"]
File Changes1. tests/BotSharp.Plugin.AgentSkills.Tests/Hooks/AgentSkillsHooksPropertyTests.cs
|
Contributor
Code Review by Qodo
1.
|
Contributor
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces an Agent Skills capability across the BotSharp solution: adding a new AgentSkills plugin (skill discovery, instruction injection, and tool callbacks), persisting agent-selected skills in storage, and providing sample skills + a new test project to validate the implementation. It also adds an Aspire settings file for tooling.
Changes:
- Add a new
BotSharp.Plugin.AgentSkillsplugin (services, hooks, functions, settings, docs) and wire it into WebStarter. - Extend agent persistence (file + Mongo) and OpenAPI agent creation to include
Skills. - Add test skill fixtures and a new
BotSharp.Plugin.AgentSkills.Testsproject with unit/integration/property tests.
Reviewed changes
Copilot reviewed 96 out of 97 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
.aspire/settings.json |
Adds Aspire configuration pointing tools to the AppHost project. |
Directory.Packages.props |
Updates/introduces centralized package versions needed for the new plugin/test project. |
src/Infrastructure/BotSharp.Abstraction/Agents/Enums/AgentField.cs |
Adds Skills to the updatable agent fields enum. |
src/Infrastructure/BotSharp.Abstraction/Agents/Models/Agent.cs |
Adds Agent.Skills and cloning/setter support. |
src/Infrastructure/BotSharp.Abstraction/Agents/Models/AgentSkill.cs |
Introduces the AgentSkill domain model. |
src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.UpdateAgent.cs |
Ensures agent update pipeline copies Skills. |
src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.Agent.cs |
Adds persistence/update support for agent Skills in file repository. |
src/Infrastructure/BotSharp.OpenAPI/ViewModels/Agents/Request/AgentCreationModel.cs |
Adds Skills to agent creation request model and mapping. |
src/Plugins/BotSharp.Plugin.AgentSkills/AgentSkillsPlugin.cs |
Registers AgentSkills plugin DI services, hooks, and function callbacks. |
src/Plugins/BotSharp.Plugin.AgentSkills/BotSharp.Plugin.AgentSkills.csproj |
Adds the AgentSkills plugin project definition and packaged content. |
src/Plugins/BotSharp.Plugin.AgentSkills/CHANGELOG.md |
Documents the plugin refactor/features. |
src/Plugins/BotSharp.Plugin.AgentSkills/Controllers/SkillController.cs |
Adds /skills endpoint for listing loaded skills. |
src/Plugins/BotSharp.Plugin.AgentSkills/Functions/AIToolCallbackAdapter.cs |
Contains (currently commented) adapter scaffold for AIFunction → IFunctionCallback. |
src/Plugins/BotSharp.Plugin.AgentSkills/Functions/GetInstructionsFn.cs |
Implements get-available-skills callback. |
src/Plugins/BotSharp.Plugin.AgentSkills/Functions/GetSkillBynameFn.cs |
Implements get-skill-by-name callback. |
src/Plugins/BotSharp.Plugin.AgentSkills/Functions/GetSkillFileContentFn.cs |
Implements read-skill-file-content callback. |
src/Plugins/BotSharp.Plugin.AgentSkills/Hooks/AgentSkillsFunctionHook.cs |
Registers skill-related tools during function loading. |
src/Plugins/BotSharp.Plugin.AgentSkills/Hooks/AgentSkillsInstructionHook.cs |
Injects skill metadata into agent instructions (with agent type filtering). |
src/Plugins/BotSharp.Plugin.AgentSkills/MIGRATION.md |
Migration guide for adopting the new plugin/config/structure. |
src/Plugins/BotSharp.Plugin.AgentSkills/Services/ISkillService.cs |
Defines skill service API used by hooks and functions. |
src/Plugins/BotSharp.Plugin.AgentSkills/Services/SkillService.cs |
Implements skill discovery/loading and instruction generation. |
src/Plugins/BotSharp.Plugin.AgentSkills/Settings/AgentSkillsSettings.cs |
Adds configuration settings and validation for AgentSkills. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkill.cs |
Implements skill parsing/validation/definition generation. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillAsToolOptions.cs |
Options controlling skill definition generation. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillReader.cs |
Reads SKILL.md and enumerates skill resources (scripts/references/assets). |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillValidationResult.cs |
Validation result DTO for skill validation. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkills.cs |
Represents a skill set and builds <available_skills> instructions XML. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillsAsToolsOptions.cs |
Options describing generated tool naming/availability. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillsFactory.cs |
Finds skills on disk and applies validation/filter rules. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillsOptions.cs |
Configuration options for skill discovery filtering/validation. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/AgentSkillsOptionsValidationRule.cs |
Defines strict/loose/none validation policies. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/GetSkillArgs.cs |
Defines tool args for get-skill-by-name. |
src/Plugins/BotSharp.Plugin.AgentSkills/Skills/SkillFileContentArgs.cs |
Defines tool args for read-skill-file-content. |
src/Plugins/BotSharp.Plugin.AgentSkills/Using.cs |
Adds global usings for the plugin. |
src/Plugins/BotSharp.Plugin.AgentSkills/data/agents/471ca181-375f-b16f-7134-5f868ecd31c6/agent.json |
Adds an AgentSkills “skill” agent with configured skills. |
src/Plugins/BotSharp.Plugin.AgentSkills/data/agents/471ca181-375f-b16f-7134-5f868ecd31c6/instructions/instruction.liquid |
Adds progressive-disclosure instruction content for skills usage. |
src/Plugins/BotSharp.Plugin.MongoStorage/Collections/AgentDocument.cs |
Adds Mongo agent document storage for Skills. |
src/Plugins/BotSharp.Plugin.MongoStorage/Models/AgentSkillMongoElement.cs |
Adds Mongo DTO and mapping helpers for agent skills. |
src/Plugins/BotSharp.Plugin.MongoStorage/Repository/MongoRepository.Agent.cs |
Adds skills update path and includes skills in “update all fields”. |
src/WebStarter/WebStarter.csproj |
Adds project reference to the AgentSkills plugin. |
src/WebStarter/appsettings.json |
Enables AgentSkills plugin loading and adds AgentSkills configuration section. |
src/WebStarter/AgentSkills/data-analysis/SKILL.md |
Adds a sample “data-analysis” skill documentation. |
src/WebStarter/AgentSkills/data-analysis/assets/analysis_config.json |
Adds config asset for data analysis skill. |
src/WebStarter/AgentSkills/data-analysis/scripts/analyze_csv.py |
Adds example analysis script (placeholder implementation). |
src/WebStarter/AgentSkills/data-analysis/scripts/clean_data.py |
Adds example cleaning script (placeholder implementation). |
src/WebStarter/AgentSkills/data-analysis/scripts/visualize_data.py |
Adds example visualization script (placeholder implementation). |
src/WebStarter/AgentSkills/employee-handbook/SKILL.md |
Adds employee handbook skill entry and reference links. |
src/WebStarter/AgentSkills/employee-handbook/references/Benefits.md |
Adds handbook reference doc. |
src/WebStarter/AgentSkills/employee-handbook/references/CultureAndValues.md |
Adds handbook reference doc. |
src/WebStarter/AgentSkills/employee-handbook/references/HoursAndAttendance.md |
Adds handbook reference doc. |
src/WebStarter/AgentSkills/employee-handbook/references/Pay.md |
Adds handbook reference doc. |
src/WebStarter/AgentSkills/pdf-processing/SKILL.md |
Adds a sample “pdf-processing” skill documentation. |
src/WebStarter/AgentSkills/pdf-processing/assets/config.json |
Adds config asset for pdf-processing skill. |
src/WebStarter/AgentSkills/pdf-processing/references/pdf_processing_guide.md |
Adds detailed reference documentation for pdf-processing skill. |
src/WebStarter/AgentSkills/pdf-processing/scripts/extract_tables.py |
Adds example table extraction script (placeholder implementation). |
src/WebStarter/AgentSkills/pdf-processing/scripts/extract_text.py |
Adds example text extraction script (placeholder implementation). |
src/WebStarter/AgentSkills/pdf-processing/scripts/get_metadata.py |
Adds example metadata extraction script (placeholder implementation). |
src/WebStarter/AgentSkills/secret-formulas/SKILL.md |
Adds a “secret-formulas” skill stub referencing a script. |
src/WebStarter/AgentSkills/secret-formulas/scripts/TheExtraSecretFormula.py |
Adds a simple python script for the “secret-formulas” skill. |
src/WebStarter/AgentSkills/speak-like-a-pirate/SKILL.md |
Adds a persona-style skill definition. |
src/WebStarter/AgentSkills/web-scraping/SKILL.md |
Adds a sample “web-scraping” skill documentation. |
src/WebStarter/AgentSkills/web-scraping/assets/scraping_config.json |
Adds config asset for web-scraping skill. |
src/WebStarter/AgentSkills/web-scraping/scripts/download_images.py |
Adds example image download script (placeholder implementation). |
src/WebStarter/AgentSkills/web-scraping/scripts/scrape_page.py |
Adds example page scraping script (placeholder implementation). |
src/WebStarter/AgentSkills/web-scraping/scripts/scrape_table.py |
Adds example table scraping script (placeholder implementation). |
tests/BotSharp.Plugin.AgentSkills.Tests/BotSharp.Plugin.AgentSkills.Tests.csproj |
Adds a new test project for AgentSkills plugin. |
tests/BotSharp.Plugin.AgentSkills.Tests/Functions/AIToolCallbackAdapterTests.cs |
Adds tests for the (currently missing/commented) adapter. |
tests/BotSharp.Plugin.AgentSkills.Tests/Helpers/TestLogger.cs |
Adds a simple test logger. |
tests/BotSharp.Plugin.AgentSkills.Tests/Hooks/PROPERTY_TESTS_README.md |
Documents property-based tests approach for hooks. |
tests/BotSharp.Plugin.AgentSkills.Tests/README.md |
Documents how to run and structure AgentSkills tests. |
tests/BotSharp.Plugin.AgentSkills.Tests/Services/SkillServicePropertyTests.cs |
Adds property-based tests for the skill service. |
tests/BotSharp.Plugin.AgentSkills.Tests/Services/SkillServiceTests.cs |
Adds integration-style tests for the skill service. |
tests/BotSharp.Plugin.AgentSkills.Tests/Settings/AgentSkillsSettingsTests.cs |
Adds unit tests for settings binding and validation. |
tests/BotSharp.Plugin.AgentSkills.Tests/TestBase.cs |
Adds shared test setup/DI base class. |
tests/BotSharp.Plugin.AgentSkills.Tests/TestSetupTests.cs |
Adds tests verifying fixture directories/files exist. |
tests/BotSharp.Plugin.AgentSkills.Tests/Usings.cs |
Adds global usings for the test project. |
tests/BotSharp.Plugin.AgentSkills.Tests/appsettings.test.json |
Adds test settings including AgentSkills section. |
tests/test-skills/README.md |
Adds documentation describing test skill fixtures and their intended usage. |
tests/test-skills/minimal-skill/SKILL.md |
Adds minimal skill fixture. |
tests/test-skills/skill-with-scripts/SKILL.md |
Adds script-focused skill fixture. |
tests/test-skills/skill-with-scripts/scripts/data_processor.py |
Adds python script fixture. |
tests/test-skills/skill-with-scripts/scripts/file_analyzer.py |
Adds python script fixture. |
tests/test-skills/skill-with-scripts/scripts/file_operations.sh |
Adds bash script fixture. |
tests/test-skills/skill-with-scripts/scripts/system_info.sh |
Adds bash script fixture. |
tests/test-skills/valid-skill/SKILL.md |
Adds full-featured skill fixture. |
tests/test-skills/valid-skill/assets/config.json |
Adds asset fixture. |
tests/test-skills/valid-skill/assets/template.txt |
Adds asset fixture. |
tests/test-skills/valid-skill/references/api_reference.md |
Adds reference doc fixture. |
tests/test-skills/valid-skill/references/workflow.md |
Adds reference doc fixture. |
tests/test-skills/valid-skill/scripts/test_script.py |
Adds python script fixture. |
tests/test-skills/valid-skill/scripts/test_script.sh |
Adds bash script fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+49
to
+55
| if (!allowedFiles.Contains(filePath)) | ||
| { | ||
| contents = $"Error: File '{filePath}' is not a valid Skill-file"; | ||
| } | ||
|
|
||
| contents = File.ReadAllText(args.FilePath, Encoding.UTF8); | ||
| message.Content = contents; |
There was a problem hiding this comment.
Security/logic issue: even when the requested file is not in allowedFiles, the code still unconditionally reads it (File.ReadAllText(args.FilePath, ...)) and returns its contents. This bypasses the allow-list check and can expose arbitrary files if filepath is attacker-controlled. Return early on disallowed paths, and only read after validating the resolved path matches an allowed skill file (ideally normalize/resolve and compare full paths).
Comment on lines
+156
to
+180
| if (optionsToUse.IncludeLicenseInformation && string.IsNullOrWhiteSpace(License)) | ||
| { | ||
| builder.AppendLine($"<license>{License}</license>"); | ||
| } | ||
|
|
||
| if (optionsToUse.IncludeCompatibilityInformation && string.IsNullOrWhiteSpace(Compatibility)) | ||
| { | ||
| builder.AppendLine($"<compatibility>{Compatibility}</compatibility>"); | ||
| } | ||
|
|
||
| if (optionsToUse.IncludeMetadata && Metadata?.Count > 0) | ||
| { | ||
| builder.AppendLine("<metadata>"); | ||
| foreach (KeyValuePair<string, string> keyValuePair in Metadata) | ||
| { | ||
| builder.AppendLine($"<{keyValuePair.Key}>{keyValuePair.Value}</{keyValuePair.Key}>"); | ||
| } | ||
|
|
||
| builder.AppendLine("</metadata>"); | ||
| } | ||
|
|
||
| if (optionsToUse.IncludeAllowedTools && string.IsNullOrWhiteSpace(AllowedTools)) | ||
| { | ||
| builder.AppendLine($"<allowedTools>{AllowedTools}</allowedTools>"); | ||
| } |
There was a problem hiding this comment.
The conditional checks for optional fields are inverted: IncludeLicenseInformation/IncludeCompatibilityInformation/IncludeAllowedTools only emit tags when the value is whitespace. This results in empty elements and skips output when values are actually present. Flip these checks to emit only when the corresponding value is non-empty.
Comment on lines
+99
to
+103
| functions.Add(new FunctionDef | ||
| { | ||
| Name = "GetInstructionsFn", | ||
| Description = $"Get a list of the available skills" | ||
| }); |
There was a problem hiding this comment.
Function registration mismatch: you register a FunctionDef named GetInstructionsFn, but the corresponding callback GetInstructionsFn advertises Name => "get-available-skills". FunctionDef.Name must match the callback Name for the tool to be invokable. Rename the FunctionDef to get-available-skills (and keep naming consistent with the other tools).
Comment on lines
+52
to
+59
| var settings = CreateSettings(); | ||
|
|
||
| // Act - Load skills multiple times | ||
| var service1 = new SkillService(_factory, settings, _logger); | ||
| var count1 = service1.GetSkillCount(); | ||
| var instructions1 = service1.GetInstructions(); | ||
| var tools1 = service1.GetTools(); | ||
|
|
There was a problem hiding this comment.
These property tests construct SkillService(_factory, settings, _logger) and call GetInstructions()/GetTools(), but the current SkillService requires an IServiceProvider and only exposes GetInstructions(Agent). As written, this test project will not compile; refactor the test harness to use the actual service signature (DI + a test Agent) or adjust the service API.
Comment on lines
+43
to
+52
| IEnumerable<string> allowedFiles = Skills.SelectMany(x => x.AssetFiles.Union(x.OtherFiles).Union(x.ScriptFiles).Union(x.ReferenceFiles)); | ||
|
|
||
| var args = JsonSerializer.Deserialize<SkillFileContentArgs>(message.FunctionArgs) ?? new(); | ||
|
|
||
| var filePath = args.FilePath; | ||
| var contents = string.Empty; | ||
| if (!allowedFiles.Contains(filePath)) | ||
| { | ||
| contents = $"Error: File '{filePath}' is not a valid Skill-file"; | ||
| } |
There was a problem hiding this comment.
allowedFiles contains the full paths returned by Directory.GetFiles(...), but the tool argument is named filepath and is likely to be provided as a relative path (as described in SKILL.md examples). Comparing allowedFiles.Contains(filePath) will fail in normal use and is case-sensitive; normalize to full paths (e.g., combine with skill root and resolve) and compare with an ordinal-ignore-case comparer on Windows.
Comment on lines
+16
to
+20
| /// <summary> | ||
| /// Name of the Skill | ||
| /// < | ||
| public required string Name { get; set; } | ||
|
|
There was a problem hiding this comment.
The XML doc comment for Name is malformed (/// < without closing tags), which will break generated XML documentation and can produce warnings/errors when GenerateDocumentationFile is enabled. Fix the summary comment block to be well-formed.
Comment on lines
+1
to
+3
| { | ||
| "appHostPath": "..\\src\\BotSharp.AppHost\\BotSharp.AppHost.csproj" | ||
| } No newline at end of file |
There was a problem hiding this comment.
PR description says the change only adds .aspire/settings.json, but this PR also introduces a large AgentSkills plugin implementation, new test projects/assets, and storage model changes. Update the PR description to accurately reflect the scope, or split into separate PRs to keep reviews manageable.
Comment on lines
+173
to
+184
| if (!agentskills.Any(s => s.Name.Equals(skill.Name, StringComparison.OrdinalIgnoreCase))) | ||
| { | ||
| _logger.LogWarning("Agent {AgentName} has skill '{Skill}' which is not available in loaded skills", agent.Name, skill); | ||
| } | ||
| else | ||
| { | ||
| var agentSkill = agentskills.FirstOrDefault(s => s.Name== skill.Name); | ||
| availableSkillToolBuilder.AppendLine("\t<skill>"); | ||
| availableSkillToolBuilder.AppendLine($"\t\t<name>{agentSkill.Name}</name>"); | ||
| availableSkillToolBuilder.AppendLine($"\t\t<description>{agentSkill.Description}</description>"); | ||
| availableSkillToolBuilder.AppendLine($"\t\t<location>{agentSkill.FolderPath}</location>"); | ||
| availableSkillToolBuilder.AppendLine("\t</skill>"); |
There was a problem hiding this comment.
Possible null dereference: you check skill existence with Any(...OrdinalIgnoreCase) but then fetch it with a case-sensitive predicate (FirstOrDefault(s => s.Name== skill.Name)) and immediately dereference agentSkill.Name/Description/FolderPath. If the only difference is casing, agentSkill will be null and this will throw. Use the same case-insensitive comparison when retrieving, and guard against null before dereferencing.
Comment on lines
+49
to
+53
| include = true; | ||
| break; | ||
| default: | ||
| throw new ArgumentOutOfRangeException(nameof(excludedSkillsLog)); | ||
| } |
There was a problem hiding this comment.
The default case throws ArgumentOutOfRangeException(nameof(excludedSkillsLog)), but the out-of-range value is optionsToUse.ValidationRules. Throwing with the wrong param name makes debugging harder; use the validation rule parameter/property name instead.
Comment on lines
+49
to
+75
| public void Constructor_WithValidDirectory_ShouldLoadSkills() | ||
| { | ||
| // Arrange | ||
| var settings = CreateSettings(); | ||
|
|
||
| // Act | ||
| var service = new SkillService(_factory, settings, _logger); | ||
|
|
||
| // Assert | ||
| service.Should().NotBeNull(); | ||
| var skillCount = service.GetSkillCount(); | ||
| skillCount.Should().BeGreaterThan(0, "test skills directory should contain at least one skill"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Test 3.3.2: Verify GetInstructions() returns valid XML format. | ||
| /// </summary> | ||
| [Fact] | ||
| public void GetInstructions_WithLoadedSkills_ShouldReturnValidXml() | ||
| { | ||
| // Arrange | ||
| var settings = CreateSettings(); | ||
| var service = new SkillService(_factory, settings, _logger); | ||
|
|
||
| // Act | ||
| var instructions = service.GetInstructions(); | ||
|
|
There was a problem hiding this comment.
This test uses a SkillService constructor overload and APIs (GetInstructions(), GetTools()) that don't exist on the current BotSharp.Plugin.AgentSkills.Services.SkillService implementation (it now takes IServiceProvider and GetInstructions(Agent)). Update the tests to build a DI container (register AgentSkillsSettings) and call the current public APIs, or update the service API to match the test expectations.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
src/Plugins/BotSharp.Plugin.AgentSkills/Services/SkillService.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+356
to
+359
| public Agent SetSkills(List<AgentSkill>? skills) | ||
| { | ||
| Skills = skills ?? []; | ||
| return this; |
Contributor
There was a problem hiding this comment.
2. setskills assigns shared list 📘 Rule violation ⛯ Reliability
Agent.SetSkills assigns the caller-provided list directly to Skills, allowing the caller to mutate agent state after assignment. This violates the defensive-copy requirement for caller-provided collections.
Agent Prompt
## Issue description
`Agent.SetSkills` stores the input `skills` list by reference instead of copying it.
## Issue Context
Callers can retain the reference and mutate it later, causing unexpected agent state changes.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Abstraction/Agents/Models/Agent.cs[356-359]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// </summary> | ||
| public IList<Skills.AgentSkill> GetAgentSkills() | ||
| { | ||
| return _agentSkills?.Skills ?? new List<Skills.AgentSkill>(); |
Contributor
There was a problem hiding this comment.
3. getagentskills() returns internal list 📘 Rule violation ⛯ Reliability
SkillService.GetAgentSkills() returns _agentSkills.Skills directly, exposing internal mutable state to callers. External mutation can corrupt the service's cached skill state across requests.
Agent Prompt
## Issue description
`GetAgentSkills()` exposes the internal `_agentSkills.Skills` list by reference.
## Issue Context
This creates shared mutable state where callers can modify the internal cache unintentionally.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.AgentSkills/Services/SkillService.cs[110-113]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
+55
| if (!allowedFiles.Contains(filePath)) | ||
| { | ||
| contents = $"Error: File '{filePath}' is not a valid Skill-file"; | ||
| } | ||
|
|
||
| contents = File.ReadAllText(args.FilePath, Encoding.UTF8); | ||
| message.Content = contents; |
Contributor
There was a problem hiding this comment.
4. read-skill-file-content ignores allowlist 📘 Rule violation ⛯ Reliability
GetSkillFileContentFn.Execute sets an error when the requested file is not allowed but still reads the file unconditionally, risking exceptions and unsafe behavior. This violates the requirement to add safe fallbacks at external/optional boundaries.
Agent Prompt
## Issue description
The function writes an error message for disallowed paths but still reads the file, which can throw or leak content.
## Issue Context
This is an external boundary (tool input from the model/user) and must have safe fallbacks when invalid.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.AgentSkills/Functions/GetSkillFileContentFn.cs[45-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var agentId = _routingCtx.GetCurrentAgentId(); | ||
| var agent = await _agentService.GetAgent(agentId); | ||
| var Skills = _skillService.GetAgentSkills(agent); | ||
| var args = JsonSerializer.Deserialize<GetSkillArgs>(message.FunctionArgs) ?? new(); |
Contributor
There was a problem hiding this comment.
5. functionargs null not handled 📘 Rule violation ⛯ Reliability
GetSkillBynameFn.Execute deserializes message.FunctionArgs without checking for null/empty, which can throw at runtime. This violates the requirement for null/empty guards at optional boundaries.
Agent Prompt
## Issue description
`FunctionArgs` is deserialized without checking for null/empty, which can throw exceptions.
## Issue Context
Tool calls can arrive without arguments or with invalid JSON; the handler should return a safe error message.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.AgentSkills/Functions/GetSkillBynameFn.cs[34-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+99
to
+103
| functions.Add(new FunctionDef | ||
| { | ||
| Name = "GetInstructionsFn", | ||
| Description = $"Get a list of the available skills" | ||
| }); |
Contributor
There was a problem hiding this comment.
6. getinstructionsfn name mismatch 📘 Rule violation ✓ Correctness
AgentSkillsFunctionHook registers a function named GetInstructionsFn, but the callback tool name is get-available-skills, creating a contract mismatch. This breaks consistent API/tool naming and can prevent correct tool binding.
Agent Prompt
## Issue description
The function hook registers `Name = "GetInstructionsFn"` but the callback tool name is `"get-available-skills"`, causing contract mismatch.
## Issue Context
Tool routing typically matches function definitions and callbacks by name; inconsistent naming prevents correct binding.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.AgentSkills/Hooks/AgentSkillsFunctionHook.cs[99-103]
- src/Plugins/BotSharp.Plugin.AgentSkills/Functions/GetInstructionsFn.cs[24-26]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
src/Plugins/BotSharp.Plugin.AgentSkills/Functions/AIToolCallbackAdapter.cs
Outdated
Show resolved
Hide resolved
| public List<AgentRuleMongoElement> Rules { get; set; } | ||
| public AgentLlmConfigMongoModel? LlmConfig { get; set; } | ||
|
|
||
| public List<AgentSkillMongoElement>? Skills { get; set; } |
Contributor
There was a problem hiding this comment.
8. Mongo skills dropped on read 🐞 Bug ✓ Correctness
Mongo storage adds/persists AgentDocument.Skills but TransformAgentDocument does not map it back to Agent.Skills, so skills are silently lost when agents are loaded from MongoDB. This causes agents to appear without skills despite persisted data.
Agent Prompt
### Issue description
Skills are stored in Mongo but never restored into the domain Agent model.
### Issue Context
This results in silent loss of configured skills for any agent retrieved from MongoDB.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MongoStorage/Repository/MongoRepository.Agent.cs[763-799]
- src/Plugins/BotSharp.Plugin.MongoStorage/Collections/AgentDocument.cs[27-33]
- src/Plugins/BotSharp.Plugin.MongoStorage/Models/AgentSkillMongoElement.cs[27-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Use a case-insensitive comparison when finding an AgentSkill (agentskills.FirstOrDefault now uses s.Name.Equals(skill.Name, StringComparison.OrdinalIgnoreCase)) to avoid missing matches due to casing. Also fix a validation bug in AgentSkill: the code now checks Description.Length (not Name.Length) against the 1-1024 character limit so the description length constraint is enforced correctly.
Oceania2018
approved these changes
Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds a new
.aspire/settings.jsonfile to the project, specifying the path to the application host project. This configuration helps tools locate the main application entry point.Configuration update:
.aspire/settings.jsonwith anappHostPathpointing to..\src\BotSharp.AppHost\BotSharp.AppHost.csproj.This pull request introduces a new
.aspire/settings.jsonconfiguration file, specifying the path to the application's host project. This change helps standardize project settings and improve environment setup.Configuration:
.aspire/settings.jsonto define theappHostPathpointing toBotSharp.AppHost.csproj.